Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

allow the setHash option to be overridden on a per atom set basis #35

Merged
merged 1 commit into from
Apr 25, 2024
Merged

allow the setHash option to be overridden on a per atom set basis #35

merged 1 commit into from
Apr 25, 2024

Conversation

scamden
Copy link
Contributor

@scamden scamden commented Apr 12, 2024 •
edited
Loading

This PR allows you to change how the hash is modified when you set the atom value itself instead of only being able to choose on atom creation. This is useful for things like replacing while initializing a set of atoms but then push for later changes. I added a test to check the functionality but also as an example of the use case.

I also added a debug jest script to package.json cause I find it useful but I can remove if you don't want that.

@codesandbox-ci CodeSandbox CI
Copy link

codesandbox-ci bot commented Apr 12, 2024 •
edited
Loading

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@scamden scamden force-pushed the sterling-per-set-hash-option branch from dcca0d7 to 1c137c6 Compare April 12, 2024 22:34
@scamden
Copy link
Contributor Author

scamden commented Apr 19, 2024

@Flirre any thoughts on this? the second argument is a little weird but i thought it was better than trying to change the main state parameter, wdyt?

@Flirre
Copy link
Collaborator

Flirre commented Apr 19, 2024

@scamden
First of all, I like this PR. Great functionality to have 👍
I also agree that the second argument feels a little off and I think I would like something closer to how it is setup for the atomWithLocation.
Perhaps sending in an object like this: setCount(1, {setHash: 'replaceState'});. Feels like it makes it a little clearer what you're overriding. Also leaves it open for extension if you're supposed to be able to override more things in the future.

What do you think?

Flirre
Flirre reviewed Apr 19, 2024
View reviewed changes
package.json Show resolved Hide resolved
@scamden
Copy link
Contributor Author

scamden commented Apr 19, 2024

Perhaps sending in an object like this: setCount(1, {setHash: 'replaceState'});. Feels like it makes it a little clearer what you're overriding.

i like this! and didn't realize atomWithLocation has similar pattern! also allows for adding future options (although maybe that's unlikely.) I'll make the change and push again

@scamden scamden force-pushed the sterling-per-set-hash-option branch from 1c137c6 to cfd6e4f Compare April 19, 2024 19:13
@scamden
Copy link
Contributor Author

scamden commented Apr 19, 2024

@Flirre made those changes! could you TAL again?

Flirre
Flirre reviewed Apr 21, 2024
View reviewed changes
Copy link
Collaborator

@Flirre Flirre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only minor comments left, then I think it's ready for a merge!

src/atomWithHash.ts Show resolved Hide resolved
src/atomWithHash.ts Outdated Show resolved Hide resolved
@scamden scamden force-pushed the sterling-per-set-hash-option branch from cfd6e4f to 8ef0c7e Compare April 22, 2024 23:45
@scamden
Copy link
Contributor Author

scamden commented Apr 25, 2024

@Flirre can we merge? i'm using a copy and pasted version of this in our code base until we get his in haha

@Flirre Flirre merged commit 4666841 into jotaijs:main Apr 25, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Flirre Flirre Flirre left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@scamden @Flirre